fix(healthcheck): reuse the checker on node-only upstream changes instead of destroy-and-rebuild#13629
fix(healthcheck): reuse the checker on node-only upstream changes instead of destroy-and-rebuild#13629AlinsRan wants to merge 22 commits into
Conversation
When an upstream's nodes change but its `checks` config is unchanged, the health-check manager destroyed the existing checker and built a new one. Two problems followed: 1. fetch_checker() keys the working checker by a version derived from both modifiedIndex and the nodes version, so a node-only change makes it return nil until the timer rebuilds the checker. During that window api_ctx.up_checker is nil and the balancer routes traffic to nodes already known to be unhealthy (apache#13282). 2. The rebuild throws away the checker's accumulated health state and re-probes every node from scratch. The manager now reconciles the existing checker's targets in place with add_target/remove_target when the `checks` config is unchanged (compared with core.table.deep_eq), keeping the checker and its state alive. timer_working_pool_check no longer destroys a checker for a node-only version change, and when a rebuild is genuinely required (the `checks` config changed) the new checker is created and inserted into the working pool before the old one is released, so fetch_checker never observes a nil gap. Bumps the lua-resty-healthcheck-api7 rockspec dependency to 3.3.0-0, which contains the companion library fix (clean every checker each window + release the periodic lock when idle) required by this change. Adds t/node/healthcheck-incremental-update.t: a node-only change must not destroy/rebuild the checker (no "clear checker"), while a checks-config change still rebuilds it.
When the checks config changes, install the freshly created checker into the working pool before stopping the previous one. This prevents a request from briefly fetching a stopped checker for the old version during the swap window.
…rop to 0 - compute_targets now returns an ordered array (preserving node order) so targets are added deterministically, keeping ordered error-log assertions stable. - Only keep/reuse a checker incrementally when the upstream still has nodes; when the node count drops to 0 the checker is destroyed as before. - Update existing healthcheck tests to assert the new incremental-reuse behaviour (a checker is reused instead of recreated when only the nodes change, and is not cleared in that case).
… config The incremental-reuse reconcile runs in timer_create_checker, which only acts on resources placed into the waiting pool by fetch_checker (i.e. when a request is routed). Re-route a request after the second PUT so the manager observes the new version and logs the incremental reuse.
dosubot
Bot
added
size:L
… independent of the node-only incremental-update path (apache#13629)
…al sync failures - resty.healthcheck keys targets by ip+port+hostname (the Host header is not part of the identity). Remove stale targets before adding new ones so a Host-header-only change (pass_host/upstream_host) re-applies the new header instead of add_target() being a no-op on the still-present old target and the subsequent remove dropping the live target. - sync_checker_targets now returns whether every add/remove succeeded; the caller only commits the reused checker for the new version when the sync fully succeeds, otherwise it falls through to a full rebuild so the upstream still converges instead of being stuck on a stale target set.
membphis
left a comment
membphis
left a comment
There was a problem hiding this comment.
This rebuild order is unsafe when the checks config changes while the target identity stays the same. The new checker is created and published first, so its add_target() calls reuse the existing shared shm target list entries for the same ip:port:hostname. Then the old checker calls delayed_clear(), which marks that same shared target list for purge. After the cleanup window, the targets that the newly published checker still depends on can be removed.
This blocks merge because checks-only updates can make active health-check targets disappear after DELAYED_CLEAR_TIMEOUT, especially once the cleanup fix in #13627 makes stale-target purging reliable. Please reorder the swap so delayed_clear() happens before the new checker re-adds targets, or otherwise ensure the new checker clears any inherited purge_time before it is considered published. It would also be good to add a regression test that waits past the cleanup window after a checks-config rebuild and verifies /v1/healthcheck still contains the live targets.
… are not purged On a checks-config change the manager rebuilds the checker. The new checker shares the old one's shm target list (same healthchecker name). add_target() only un-marks a target's purge_time when the target is re-added *after* being marked, so doing create_checker() (re-add) before the old checker's delayed_clear() left the surviving targets marked with a purge_time that nobody cleared -- the cleanup then purged the live checker's own targets once the delayed-clear window elapsed. Run delayed_clear() before create_checker() so the re-add un-marks surviving targets while genuinely dropped ones keep their purge_time. The old checker is still only stopped after the new one is published, so there is no rebuild gap. Add a regression test asserting the surviving targets remain in /v1/healthcheck after the delayed-clear window following a checks-config change.
|
This still leaves the health-filter bypass window on node-only updates. The PR keeps the old checker alive and reconciles targets in Trigger:
This blocks the fix for #13282 because the health state is preserved internally but is not used by requests during the version transition. Please let |
…ransition A node-only change (discovery/DNS bumps _nodes_ver) queues the new version and timer_create_checker reconciles the existing checker in place. But fetch_checker only returned a checker on an exact version match, so during the ~1s until the timer ran it returned nil, api_ctx.up_checker was nil, and the balancer fell back to all upstream nodes -- a node already known unhealthy could take traffic during the transition (the request-path side of apache#13282). Return the existing live (non-dead) checker for the resource while the new version is queued for reconciliation, so requests keep using the preserved health state throughout the transition. Regression test: mark a node unhealthy, apply a node-only update, and assert a burst of requests in the transition window never hit the unhealthy node.
|
Good catch — confirmed. Fixed in 3688210: One deviation from your suggestion worth flagging: I return the live checker whenever one exists (and isn't dead), rather than gating on reuse-eligibility ( Regression test ( |
|
I hit an issue while testing these changes. Setup: APISIX container (with 2 NGINX worker processes) and 2 upstream nodes. Steps:
Potential fix: Update |
…ulti-worker With multiple workers, a peer worker can create the checker and populate the shared shm target list, after which a node is removed from the upstream. A worker that never had the checker in its own working_pool then reaches create_checker(), which only added targets -- so the removed node lingered in the shm and kept being probed and returned by /v1/healthcheck. create_checker() now also removes targets present in the shm but absent from the config, reconciling it to the desired set. The add pass is kept as-is (re-adding un-marks any delayed_clear purge_time, preserving the checks-rebuild fix), so a straight switch to sync_checker_targets() -- which only adds missing targets -- is deliberately avoided. Regression test seeds a stale target into the shm and asserts create_checker() drops it. Reported-by: arunmat27
dosubot
Bot
added
size:XL
…ns it When only the checks config changes, a worker that did not serve traffic keeps its old-version checker in working_pool; timer_working_pool_check then destroys that stale local handle. Because the checker's shm target list is shared by name, a same-name replacement checker (built by whichever worker serves traffic) owns it. Unconditionally calling delayed_clear() on destroy marks that live checker's targets for purge on every worker. Only tear down the shm when no same-name checker will own it (resource deleted, or new config has no checks/nodes); otherwise drop the local handle with stop() and leave the shm to the replacement checker.
Add regression coverage for more real-world multi-worker scenarios: - recreating the same upstream id within the delayed-clear window keeps its targets (re-add un-marks the pending purge_time) - deleting an upstream cleans its shm target list - a new 2-worker test asserting the shared shm target set converges to the desired node set under node churn (incremental) and a checks-config change (rebuild), with no orphan and no purge of surviving targets
… the log Replace the blanket `--- ignore_error_log` with a targeted `--- no_error_log` that asserts the health-check timer and target-reconcile paths do not log an error. The timers are pcall-wrapped, so a Lua fault in the code under test was previously logged and swallowed while the test still passed on response_body alone. Targeting specific patterns keeps the missing-optional-dependency noise from failing the run while still catching a real crash. Also note in the multi-worker test that it is a convergence/non-regression check, not a deterministic reproduction of the cross-worker asymmetry.
Follow-up to the error-log hardening: convert the one remaining blanket `--- ignore_error_log` (the checks-config rebuild test) to the same targeted `--- no_error_log`, and add `failed to create healthcheck` to the pattern set so a healthcheck.new() failure on the create_checker path is also caught.
…rkers Add a 2-worker health-status test: a dead node (1970) alongside a healthy one (1980) with active checks. Assert the shared shm reports 1970 unhealthy and, once every worker's status cache has converged, retries=0 traffic never lands on 1970 -- proving both workers filter it. This covers health STATUS propagation, not just target membership.
dosubot
Bot
added
size:XXL
…leeps The strict zero-error assertion previously relied on two fixed sleeps to let every worker's per-worker status cache converge. Under loaded CI a worker that built its checker late might not have received the unhealthy event by the burst, flaking. Drive traffic until several consecutive zero-error bursts confirm both workers filter the dead node (bounded); a genuine per-worker miss never converges and fails the test deterministically.
Good catch, and thanks for the clear repro — confirmed. A worker that never held the checker reaches On the suggested
Instead |
Three review fixes: - create_checker() removed stale shm targets AFTER adding. resty.healthcheck keys a target by ip+port+hostname (not the Host header), so a Host-header-only change made add_target a no-op on the existing identity and the following remove_target wiped the still-desired target. Remove-before-add (mirroring sync_checker_targets) fixes it; the add-all still runs last so it un-marks purge_time on survivors after a rebuild. Covered by TEST 9 (fails before). - sync_checker_targets() added targets by iterating pairs(desired) (hash order); iterate the ordered compute_targets list so adds stay deterministic. - the 2-worker test's drive() ignored request results; assert they succeed so a silent upstream failure cannot let the shm assertions pass vacuously.
The diff key was ip:port:hostheader, but resty.healthcheck keys a target by ip+port+hostname (checks.active.host maps to that hostname). When only checks.active.host changed, the old and new targets hashed to the same key, so the stale entry was not removed -- it kept being probed and reported (and in a cold-start multi-worker create_checker, where there is no delayed_clear to purge it, the leak was permanent). Include the identity hostname in the key so the diff tracks the real identity plus the Host header. Covered by TEST 10 (fails before: hostnames new-host,old-host). The hostheader stays in the key too: it is a mutable non-identity attribute that add_target cannot update in place, so a header-only change must be detected to force a remove+add (TEST 9).
…ffic timer_working_pool_check keeps the checker alive on a nodes-only change, but the actual target reconcile runs in timer_create_checker, which only drains waiting_pool -- and waiting_pool is filled by fetch_checker on the request path. So a nodes-only update on an upstream that then gets no traffic kept probing and reporting the old node set indefinitely. Enqueue the new version from timer_working_pool_check (a per-worker timer) so the reconcile is driven by the timer, not by a request. Covered by TEST 11 (fails before: ports 1980,1981).
…p 0-node rebuild Two hardening fixes in timer_create_checker (review feedback): - On the rebuild path, if create_checker() returns nil (upstream healthcheck disabled or healthcheck.new errored), the old checker was already delayed_clear'd but left in working_pool without being marked dead, so fetch_checker() (which only checks .dead) kept handing out a stopped, cleared checker until timer_working_pool_check reclaimed it ~1s later. Mark it dead, stop it and drop it from the working pool immediately. - Skip the rebuild entirely when the upstream has no nodes: there is nothing to health-check, so don't build an empty checker only for timer_working_pool_check to destroy it -- consistent with its node-count-drops-to-0 destroy behavior.
…ange timer_working_pool_check destroys the local handle on a checks-config change but, to avoid purging a peer worker's live shm targets, does not clear the shm when a replacement checker is still wanted. Previously it relied on some worker serving traffic to rebuild. If every worker went cold, nothing rebuilt and the old shm targets were left neither probed nor purged. Enqueue the new version so timer_create_checker rebuilds on the per-worker timer (symmetric with the nodes-only branch); its create_checker reconciles the shm. Covered by TEST 12 (fails before: ports 1980,1981). Also scope a misleading rebuild-gap comment and document why sync_checker_targets need not un-mark purge_time.
timer_working_pool_check destroyed the checker whenever the checks config changed (deep_eq false), blanking working_pool and only enqueueing a rebuild. Until the next timer_create_checker tick fetch_checker returned nil and health filtering was bypassed -- the same nil window this PR closes for nodes-only changes, just narrowed to checks changes. Keep the checker alive whenever the upstream still defines checks and has at least one node, and let timer_create_checker transition it (incremental reconcile for a nodes-only change, build-new-before-stop-old rebuild for a checks change). This subsumes the has_live_replacement gate, so drop it. Also address review nits: remove a redundant node-count guard already ensured by the 0-node guard, log when a checker is dropped after create_checker fails, trim narrative comments, and fix a test title.
Since the checks-config change now keeps the old checker alive and rebuilds via timer_create_checker (build-new-before-stop-old) instead of destroying it in timer_working_pool_check, an update logs "create new checker" + "releasing existing checker" rather than "try to release checker". Match the new sequence and include "releasing existing checker" in the grep.
5 participants
Description
Fixes #13282: the health-check manager destroys and rebuilds the checker on every upstream change, even when only the upstream nodes changed and the
checksconfig is identical. The rebuild opens a window where health filtering is bypassed, and — because a checker's target list lives in a shm shared by name across all workers — the per-worker destroy/rebuild also races and corrupts state across NGINX workers.Root cause
fetch_checker()keys the working checker by a version derived from bothmodifiedIndexand the nodes version. A node-only change bumps that version, so the checker is queued for an asynchronous rebuild. During that window:fetch_checker()returnednil, soapi_ctx.up_checkerwasniland the balancer's health filtering was bypassed — traffic flowed to nodes already known unhealthy for ~1-2s.timer_working_pool_checkalso destroyed the checker on any version mismatch, racingtimer_create_checkerand widening the gap.Fix — incremental reconcile
sync_checker_targets(): when thechecksconfig is unchanged (core.table.deep_eq) and only the nodes changed, reconcile the existing checker's targets in place viaremove_target/add_targetagainst the authoritative shm target list — keeping the checker and its accumulated health state alive instead of rebuilding.timer_working_pool_checkno longer destroys a checker for a node-only version change (it destroys only when the resource is gone or the node count drops to 0).checksconfig changed), the new checker is created and published before the old one is released, eliminating theup_checker == nilgap.checksconfig so node-only vs checks changes can be told apart.Fix — multi-worker correctness
A checker's target membership lives in a shm keyed only by checker name (
upstream#<resource_key>), shared by every worker, whileworking_pool/waiting_poolare per-worker and populated request-driven — so workers hold asymmetric views and a per-worker lifecycle op mutates shared state. Several fixes close the gaps this opens (surfaced in review):fetch_checker()returns the live checker during a version transition instead ofnil, so requests keep filtering unhealthy nodes across the ~1s reconcile window (closes root-cause change: added doc of how to load plugin. #1 on the request path).create_checker()reconciles the shm — a worker that never held the checker removes stale targets (not just adds), so a node removed after a peer worker populated the shm stops being probed / reported by/v1/healthcheck. Removal runs before the adds: resty.healthcheck keys a target byip+port+hostname(not the Host header), so a Host-header-only change must free the old identity first — otherwiseadd_targetis a no-op on the existing identity and the followingremove_targetwipes the still-desired target.timer_working_pool_checkclears the shared shm only when no same-name replacement will own it (resource deleted, or new config has no checks/nodes). When the config still has checks and nodes, a replacement checker built by whichever worker serves traffic owns the shm, so the destroying worker drops only its local handle (stop) — it must notdelayed_clear, which would purge the peer's live targets on every worker.Trade-off: after a
checkschange, a worker that never serves that resource again leaves the (unmarked) old targets in the shm until traffic resumes andcreate_checkerreconciles them — they are not probed in the meantime, and this is strictly safer than the eager clear that caused the cross-worker purge.Checker lifecycle after this change
fetch_checkerenqueues towaiting_pool→timer_create_checker(no existing checker)healthcheck.new()→ remove-stale →add_targetper nodetimer_create_checkerreuse branchsync_checker_targets:remove_target/add_targeton the delta only, nodelayed_clearcheckschangedtimer_create_checkerrebuild branchdelayed_clear(old)→new()+add_target(new)→ publish →stop(old)timer_working_pool_check(fetch_latest_conf=nil)delayed_clear(old)+stop+working_pool[path]=nil; periodic cleanup removes the targetsOn the
checks-changed rebuild,delayed_clear(old)runs beforecreate_checker(new)on purpose: the two checkers share the same shm target list, andadd_target()only clears a target'spurge_timewhen it is re-added after being marked. Clearing first lets surviving targets get un-marked on re-add, while genuinely dropped targets keep theirpurge_timeand are cleaned up — so achecks-only update cannot make the live checker's targets disappear afterDELAYED_CLEAR_TIMEOUT.This is apisix-side only — it uses
add_target/remove_target, which already exist in the currentlua-resty-healthcheck-api7. No dependency bump is required.Tests
t/node/healthcheck-incremental-update.t:create new checkeronce, noclear checker).checks-config change must still rebuild (clear checker).checks-config rebuild.create_checkerremoves targets left stale in the shm by another worker.create_checkerapplies a Host-header-only change instead of wiping the target.t/node/healthcheck-multiworker-reconcile.t(2 workers):checksrebuild, with no orphan and no purge of survivors.Each fix is verified fail-before / pass-after locally (OpenResty 1.29.2.4).
Scope
This PR is independent of the library. The related #13385 / #13141 / #13235 are library-side bugs (stale-target cleanup and periodic-lock release) fixed in api7/lua-resty-healthcheck#55 and delivered to apisix by the dependency bump in #13627 — they are not part of this PR. (Supersedes the #13282 portion of the now-closed #13582.)
Fixes #13282.
Checklist